Skip to content

Conversation

mikechu-optimizely
Copy link
Contributor

Summary

  • Added an GraphQLManager interface and default implementation for querying qualified segments
  • GraphQLManager is also responsible for parsing response from ODP.

Test plan

  • Added specific GraphQL request and response test class

Issues

  • OASIS-8406

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Some suggestions for error handling.
I see some C# stuffs I cannot review. Let Sohail double check.

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. Some more suggestions regarding error handling.

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikechu-optimizely
Copy link
Contributor Author

LGTM

@jaeopt

Should I wait until @msohailhussain reviews?

@jaeopt
Copy link
Contributor

jaeopt commented Aug 8, 2022

LGTM

@jaeopt

Should I wait until @msohailhussain reviews?

@mikechu-optimizely Yes, let's wait for his review.

@mikechu-optimizely
Copy link
Contributor Author

Hopefully, I've not ruined your codebase with this PR @msohailhussain 😁

@mikechu-optimizely
Copy link
Contributor Author

@msohailhussain when you have a chance, this PR also needs your 👍 Thanks!

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please address.

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before merge, please discuss GetAwaiter with me.

@mikechu-optimizely mikechu-optimizely merged commit 4e04c3a into master Aug 24, 2022
@mikechu-optimizely mikechu-optimizely deleted the mike/ats-graphql branch August 24, 2022 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants